Skip to content

Conversation

@christinaholtNOAA
Copy link
Collaborator

@christinaholtNOAA christinaholtNOAA commented Dec 1, 2025

Transition from the deprecated pynio to cfgrib for wgrib2 loading into an xarray object.

The need for this change is driven by a couple of immediate requirements:

  • The version of python needs to be updated as 3.7 is no longer supported, posing a security risk. That cannot be done with this version of pynio.
  • The environment containing pynio cannot be installed from open-use conda-forge channels.

As part of this extensive upgrade, I've also included additional tooling and unittests to help ensure quality. The current coverage is set to 75%. The intent is to follow up this PR with additional tests for 100% coverage.

New tools include:

  • mypy for typechecking
  • ruff for linting (replacing pylint), formatting (new), and list sorting (new)
  • pytest-cov for a coverage report

All of these tools can be run with a new Makefile that includes helpful targets for a more automated experience with the test framework.

Copy link

@maddenp-cu maddenp-cu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more ideas, to take or leave.

I'll circle back tomorrow to your outstanding responses to my previous review.

Copy link

@maddenp-cu maddenp-cu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost exclusively copy-editing suggestions, and only for text in diff regions or nearby -- I didn't go hunting in unchanged parts of files.

In addition to these, I'll note that there's a variety of keyword vs key word, and "Skew-T" is all over the place: "skewt", "SkewT", "Skew-T", "Skew T", etc. It could be worth (maybe someday?) normalizing these to make finding related code and comments easier. Many developers will think to do case-insensitive searching, but fewer will use regexes to make whitespace (much less hyphenation) optional. Just doing due diligence by pointing this out.

Copy link

@maddenp-cu maddenp-cu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's all I've got. I'm happy to respond to any @mentions and help with anything, and none of my comments should be considered blockers, just ideas/suggestions. Heroic effort here bringing this codebase up-to-date!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants